feat: improve communication with new Selection Command Hub via background_script#398
Conversation
Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/65b033d3-5918-49ab-acd3-a98f333765b1 Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
…nd_script Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/65b033d3-5918-49ab-acd3-a98f333765b1 Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
…cript Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/65b033d3-5918-49ab-acd3-a98f333765b1 Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
|
@copilot |
Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/8f2f64d0-4c83-45d3-8ae8-8c0516661c64 Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
…tener mock Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/8f2f64d0-4c83-45d3-8ae8-8c0516661c64 Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
対応しました(コミット 変更内容:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
…-hub Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
対応しました(コミット mainブランチ(PR #395)をマージし、 解決方針:
全804テストがパスしています。 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #398 +/- ##
==========================================
+ Coverage 26.77% 27.08% +0.31%
==========================================
Files 312 315 +3
Lines 31742 31776 +34
Branches 1682 1717 +35
==========================================
+ Hits 8498 8607 +109
+ Misses 23244 23169 -75 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
概要
Options ページと新しい Selection Command Hub 間の通信を、window.open() + postMessage() 直結から、background service worker 経由の IPC / external messaging を中心とした構成へ移行する PR です。あわせて、コマンド編集画面の「変更検知」ロジックを切り出してテスト可能にしています。
懸念点
- Hub 共有フロー(
services/hub/background.ts)に未初期化変数参照(ReferenceError)になり得る重大なバグがあります。 - Hub の URL(
NEW_HUB_URLのデフォルト: pages.dev)と、manifest.jsonのcontent_scripts/externally_connectableのmatchesが不整合で、デフォルト設定のままだと機能が動かない可能性が高いです。 hubShare.tsが IPC 送信結果を待たずに成功扱いで返し、送信失敗時の未処理 Promise/rejection や UI 誤表示につながります。removeCommandの削除結果が不正確(存在しない ID でも成功扱い)です。
改善提案
- Hub の実運用ドメインを 1 つに決め、
NEW_HUB_URL/ manifest / origin 検証を揃える。 - Hub 共有フローの
tabId管理と listener の後始末を見直し(リークと race を解消)。 - Hub 連携 API のレスポンス型(成功/失敗)を一貫させる。
shareCommandToHubを async 化するか、失敗時のハンドリングを追加して UI と整合させる。
Changes:
- Hub 共有を
hubShare.ts→ IPC 経由(BgCommand.shareCommandToHub)に変更 - background 側に Hub 連携(共有/外部メッセージ)処理とテストを追加
- コマンド変更検知ロジックを
hasCommandChangedとして分離しテスト追加
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/extension/src/test/setup.ts | chrome.runtime.onMessageExternal のモックを追加 |
| packages/extension/src/services/ipc.ts | BgCommand.shareCommandToHub を追加 |
| packages/extension/src/services/hubShare.ts | Hub 共有を IPC 呼び出しに置き換え |
| packages/extension/src/services/hubShare.test.ts | 共有ロジックのテストを IPC 前提に更新 |
| packages/extension/src/services/hub/background.ts | Hub 共有(external port)/ 外部メッセージ処理を追加 |
| packages/extension/src/services/hub/background.test.ts | Hub background ロジックのテストを追加 |
| packages/extension/src/new_command_hub.tsx | 新 Hub 向け content script エントリを追加 |
| packages/extension/src/hooks/useCommandHubBridge.ts | Hub ブリッジを簡素化(installedIds の push) |
| packages/extension/src/components/option/editor/CommandEditDialog.tsx | 変更検知を helper 関数へ移譲 |
| packages/extension/src/components/option/editor/commandChangedDetector.ts | 変更検知ロジックを新規追加 |
| packages/extension/src/components/option/editor/commandChangedDetector.test.ts | 変更検知のユニットテストを追加 |
| packages/extension/src/components/commandHub/CommandHub.tsx | useCommandHubBridge 呼び出しを削除 |
| packages/extension/src/background_script.ts | Hub handler 登録・外部 listener 初期化・レスポンス形変更 |
| packages/extension/manifest.json | 新 Hub 用 content script と externally_connectable を追加 |
…emoveListener leak Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/07c9bfe1-1e50-452b-a1a7-2fbe147c45fc Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
…ipt.ts Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/4e5a50d5-b06a-4da9-843f-82bba728b6dd Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
…ndling to addCommand Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/4e5a50d5-b06a-4da9-843f-82bba728b6dd Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
…Connect usage Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/4e5a50d5-b06a-4da9-843f-82bba728b6dd Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
コードレビュー — PR #398このPRは、Selection Command Hub との通信を 🔴 クリティカル1.
|
|
@copilot Ipc.callListener<{ command: string }, boolean>(BgCommand.addCommand, { command })
function onMessageExternal(...): boolean { const hubOrigin = new URL(NEW_HUB_URL).origin Ipc.send(BgCommand.shareCommandToHub, input) void Ipc.send(BgCommand.shareCommandToHub, input).catch((err) => { |
... 修正対象のリストが空のようです。修正してほしいClaudeのレビューコメントを教えていただけますか? |
|
@copilot
hubOrigin がリクエストごとに再計算されている const hubOrigin = new URL(NEW_HUB_URL).origin Ipc.send(BgCommand.shareCommandToHub, input) void Ipc.send(BgCommand.shareCommandToHub, input).catch((err) => { |
コードレビュー — PR #398: Selection Command Hub との通信改善概要
良い点
問題点・改善提案🔴 重要1. console.log("[onMessageExternal] Received message:", message)本番コードに 2. 型の不整合: // AddCommand
Ipc.callListener<{ command: string }, boolean>(BgCommand.addCommand, { command })
// DeleteCommand
Ipc.callListener<{ id: string }, boolean>(BgCommand.removeCommand, { id })戻り値の型が 🟡 中程度3. window.postMessage(
{ action: "SyncInstalledCommand", installedIds: ids },
hubOrigin,
)新Hub側は 4. externally_connectable: {
...manifest.externally_connectable,
matches: manifest.externally_connectable.matches.filter(...)
}
5. 🟢 軽微6. response({ result: true, install_id: clientId })PR説明では 7. if (retries > MAX_RETRIES) {
cleanup()
console.error("[Hub] Hub page did not respond to share-command in time.")
return // response() が呼ばれない
}最大リトライ超過時に 8. テストの vi.mocked(Ipc.callListener).mockResolvedValue({ result: true } as any)
vi.mocked(Storage.getCommands).mockResolvedValue([{ id: "a" }] as any)
まとめアーキテクチャの方向性は正しく、セキュリティ観点でも優れた改善です。本番リリース前に |
…rror handling Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/495d22c6-86f7-43fb-9150-1e0944f960be Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
Agent-Logs-Url: https://github.com/ujiro99/selection-command/sessions/495d22c6-86f7-43fb-9150-1e0944f960be Co-authored-by: ujiro99 <677231+ujiro99@users.noreply.github.com>
... コミット
また |
useCommandHubBridge.tsコンフリクト解決background.ts修正:onPortConnect内での未初期化tab参照 (ReferenceError) をlet tabId方式で解消background.ts修正:tab.id未取得時のremoveListenerリーク解消background.ts修正: catch ブロックでもremoveListenerを呼ぶよう対応background.ts修正:onMessageExternal失敗時のレスポンスを{ result: false, error: ... }形式に統一background_script.ts修正:removeCommandロジック整理background_script.ts修正:install_id→client_id、installId→clientIdにリネームbackground_script.ts修正:addCommandの Promise チェーンに.catch()を追加hub/background.ts修正:Ipc.callListenerの型引数を{ result: boolean; error?: string; client_id?: string }に更新hub/background.ts修正:hubOriginをモジュールトップレベルで一度だけ計算するよう変更hubShare.ts修正:void Ipc.send(...).catch(...)でエラーをログ出力するよう変更hubShare.test.ts修正:mockClear()→mockResolvedValue(true)に変更、エラーケース(SH-08)テスト追加